-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid redundant {channel,node}_announcement
signature checks
#3305
Avoid redundant {channel,node}_announcement
signature checks
#3305
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this doesn't build.
This makes cargo build
happy, but two test cases are still failing for me:
diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index e5a2f1d08..cea44eaa8 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -1722,9 +1722,9 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
pub fn update_node_from_announcement(&self, msg: &msgs::NodeAnnouncement) -> Result<(), LightningError> {
// First check if we have the announcement already to avoid the CPU cost of validating a
// redundant announcement.
- if let Some(node) = nodes.self.nodes.read().unwrap().get(&msg.node_id) {
+ if let Some(node) = self.nodes.read().unwrap().get(&msg.contents.node_id) {
if let Some(node_info) = node.announcement_info.as_ref() {
- if node_info.last_update() == msg.timestamp {
+ if node_info.last_update() == msg.contents.timestamp {
return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
}
}
@@ -1827,7 +1827,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
where
U::Target: UtxoLookup,
{
- self.pre_channel_announcement_validation_check(&msg.contents, utxo_lookup)?;
+ self.pre_channel_announcement_validation_check(&msg, utxo_lookup)?;
self.update_channel_from_unsigned_announcement_intern(msg, None, utxo_lookup)
}
@@ -1960,6 +1960,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
});
}
}
+ Ok(())
}
/// Update channel information from a received announcement.
a19a01e
to
75a9c28
Compare
🤦 juggling too many branches at once guess I forgot to test again after fixing other issues. $ git diff-tree -U3 a19a01e45 75a9c2821
diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index e5a2f1d08..4c11f4ebf 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -1722,9 +1722,9 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
pub fn update_node_from_announcement(&self, msg: &msgs::NodeAnnouncement) -> Result<(), LightningError> {
// First check if we have the announcement already to avoid the CPU cost of validating a
// redundant announcement.
- if let Some(node) = nodes.self.nodes.read().unwrap().get(&msg.node_id) {
+ if let Some(node) = self.nodes.read().unwrap().get(&msg.contents.node_id) {
if let Some(node_info) = node.announcement_info.as_ref() {
- if node_info.last_update() == msg.timestamp {
+ if node_info.last_update() == msg.contents.timestamp {
return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
}
}
@@ -1827,7 +1827,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
where
U::Target: UtxoLookup,
{
- self.pre_channel_announcement_validation_check(&msg.contents, utxo_lookup)?;
+ self.pre_channel_announcement_validation_check(&msg, utxo_lookup)?;
self.update_channel_from_unsigned_announcement_intern(msg, None, utxo_lookup)
}
@@ -1960,6 +1960,8 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
});
}
}
+
+ Ok(())
}
/// Update channel information from a received announcement.
@@ -2584,11 +2586,6 @@ pub(crate) mod tests {
};
}
- match gossip_sync.handle_node_announcement(&valid_announcement) {
- Ok(res) => assert!(res),
- Err(_) => panic!()
- };
-
let fake_msghash = hash_to_message!(zero_hash.as_byte_array());
match gossip_sync.handle_node_announcement(
&NodeAnnouncement {
@@ -2599,6 +2596,11 @@ pub(crate) mod tests {
Err(e) => assert_eq!(e.err, "Invalid signature on node_announcement message")
};
+ match gossip_sync.handle_node_announcement(&valid_announcement) {
+ Ok(res) => assert!(res),
+ Err(_) => panic!()
+ };
+
let announcement_with_data = get_signed_node_announcement(|unsigned_announcement| {
unsigned_announcement.timestamp += 1000;
unsigned_announcement.excess_data.resize(MAX_EXCESS_BYTES_FOR_RELAY + 1, 0);
@@ -2720,23 +2722,25 @@ pub(crate) mod tests {
}
}
- // Don't relay valid channels with excess data
- let valid_announcement = get_signed_channel_announcement(|unsigned_announcement| {
+ let valid_excess_data_announcement = get_signed_channel_announcement(|unsigned_announcement| {
unsigned_announcement.short_channel_id += 4;
unsigned_announcement.excess_data.resize(MAX_EXCESS_BYTES_FOR_RELAY + 1, 0);
}, node_1_privkey, node_2_privkey, &secp_ctx);
- match gossip_sync.handle_channel_announcement(&valid_announcement) {
- Ok(res) => assert!(!res),
- _ => panic!()
- };
- let mut invalid_sig_announcement = valid_announcement.clone();
+ let mut invalid_sig_announcement = valid_excess_data_announcement.clone();
invalid_sig_announcement.contents.excess_data = Vec::new();
match gossip_sync.handle_channel_announcement(&invalid_sig_announcement) {
Ok(_) => panic!(),
Err(e) => assert_eq!(e.err, "Invalid signature on channel_announcement message")
};
+ // Don't relay valid channels with excess data
+
+ match gossip_sync.handle_channel_announcement(&valid_excess_data_announcement) {
+ Ok(res) => assert!(!res),
+ _ => panic!()
+ };
+
let channel_to_itself_announcement = get_signed_channel_announcement(|_| {}, node_1_privkey, node_1_privkey, &secp_ctx);
match gossip_sync.handle_channel_announcement(&channel_to_itself_announcement) {
Ok(_) => panic!(),
$ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3305 +/- ##
==========================================
- Coverage 89.85% 89.62% -0.23%
==========================================
Files 126 126
Lines 104145 102181 -1964
Branches 104145 102181 -1964
==========================================
- Hits 93577 91582 -1995
+ Misses 7894 7877 -17
- Partials 2674 2722 +48 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If we receive `{channel,node}_announcement` messages which we already have, we first validate their signatures and then look in our graph and discover that we should discard the messages. This avoids a second lock in `node_announcement` handling but does not impact our locking in `channel_announcement` handling. It also avoids lock contention in cases where the signatures are invalid, but that should be exceedingly rare. For nodes with relatively few peers, this is a fine state to be in, however for nodes with many peers, we may see the same messages hundreds of times. This causes a rather substantial waste of CPU resources validating gossip messages. Instead, here, we change to checking our network graph first and then validate the signatures only if we don't already have the message.
75a9c28
to
56cb6a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks like the CI failures are unrelated so landing this |
a6dea2f
into
lightningdevkit:main
…edundant-gossip-validation Avoid redundant `{channel,node}_announcement` signature checks
…edundant-gossip-validation Avoid redundant `{channel,node}_announcement` signature checks
If we receive
{channel,node}_announcement
messages which we already have, we first validate their signatures and then look in our graph and discover that we should discard the messages. This avoids a second lock innode_announcement
handling but does not impact our locking inchannel_announcement
handling. It also avoids lock contention in cases where the signatures are invalid, but that should be exceedingly rare.For nodes with relatively few peers, this is a fine state to be in, however for nodes with many peers, we may see the same messages hundreds of times. This causes a rather substantial waste of CPU resources validating gossip messages.
Instead, here, we change to checking our network graph first and then validate the signatures only if we don't already have the message.